criu: enable setting of RPC config file#1892
Conversation
Reviewer's GuideAdds support for specifying a CRIU RPC configuration file (with defaults and annotations) when checkpointing/restoring containers, including tests that validate config resolution order and compatibility with different CRIU versions. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
cc: @adrianreber |
|
The correct name of the file is tricky. Not sure if we should use runc.conf or crun.conf. @rst0git You should include the annotation possibility from runc also https://github.com/opencontainers/runc/blob/main/docs%2Fcheckpoint-restore.md |
5356b8d to
47e744e
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
1 similar comment
|
Ephemeral COPR build failed. @containers/packit-build please check. |
2783d74 to
542cd62
Compare
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
_run_cr_test_with_config, the result ofrun_cr_testis ignored and the function only checks for log files, so a failing checkpoint/restore that still writes logs would incorrectly be reported as success; consider propagating therun_cr_testexit status as part of the return value. - The tests write and delete config files under
/etc/criu, which can require elevated privileges and may interfere with host configuration; it would be safer to use a test-local directory and point CRIU to it rather than mutating/etc. - The error from
handle_criu_config_fileis wrapped at the call sites with a generic"libcriu doesn't support RPC config file"message, which hides the more specific reason (e.g. CRIU version too old); consider returning or augmenting the original error text instead of overwriting it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_run_cr_test_with_config`, the result of `run_cr_test` is ignored and the function only checks for log files, so a failing checkpoint/restore that still writes logs would incorrectly be reported as success; consider propagating the `run_cr_test` exit status as part of the return value.
- The tests write and delete config files under `/etc/criu`, which can require elevated privileges and may interfere with host configuration; it would be safer to use a test-local directory and point CRIU to it rather than mutating `/etc`.
- The error from `handle_criu_config_file` is wrapped at the call sites with a generic `"libcriu doesn't support RPC config file"` message, which hides the more specific reason (e.g. CRIU version too old); consider returning or augmenting the original error text instead of overwriting it.
## Individual Comments
### Comment 1
<location> `src/libcrun/criu.c:495` </location>
<code_context>
+ config_file = CRIU_CRUN_CONFIG_FILE;
+ }
+
+ libcriu_wrapper->criu_set_config_file (config_file);
+
+ return 0;
</code_context>
<issue_to_address>
**issue (bug_risk):** The return value of criu_set_config_file is ignored, potentially hiding CRIU-side configuration errors.
`criu_set_config_file` returns an `int`, so it can fail (e.g., invalid path, permissions, CRIU errors). Ignoring this means checkpoint/restore continues as if the config was applied when it may not have been.
Please capture and return the error, e.g.:
```c
int ret = libcriu_wrapper->criu_set_config_file(config_file);
if (UNLIKELY(ret < 0))
return crun_make_error(err, 0, "failed to set CRIU config file '%s'", config_file);
```
and have callers of `handle_criu_config_file` propagate the error so config failures are visible to the caller.
</issue_to_address>
### Comment 2
<location> `tests/test_checkpoint_restore.py:283-285` </location>
<code_context>
+ raise
+
+
+def _clean_up_criu_configs():
+ for conf_file in ["crun.conf", "runc.conf", "annotation.conf"]:
+ _remove_file(os.path.join("/etc/criu", conf_file))
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider guarding tests that modify `/etc/criu` to avoid permission-related flakiness
These tests create and delete files under `/etc/criu`, which typically requires root. When run by an unprivileged user, they’ll fail with permission errors rather than exercising the intended behavior. Please either restrict them to privileged/CI runs or add a skip condition (e.g., `os.access("/etc/criu", os.W_OK)`) to avoid flaky local failures.
Suggested implementation:
```python
import errno
import pytest
```
For this guard to be effective for all tests that touch `/etc/criu`, ensure that:
1. Each test that creates/deletes `/etc/criu` config files calls `_clean_up_criu_configs()` before performing any write/cleanup under `/etc/criu`.
2. If there are other helpers that write into `/etc/criu` directly (not via `_clean_up_criu_configs()`), consider adding a similar `os.access("/etc/criu", os.W_OK)` check (and `pytest.skip`) in those helpers or at the top of the respective tests.
</issue_to_address>
### Comment 3
<location> `tests/test_checkpoint_restore.py:342-348` </location>
<code_context>
+ return _run_cr_test_with_config("crun", ("crun-dump.log", "crun-restore.log"), extra_configs=extra)
+
+
+def test_cr_with_annotation_config():
+ # Create annotation config file
+ annotations = {"org.criu.config": "/etc/criu/annotation.conf"}
+ _create_criu_config("annotation", f"log-file=annotation.log")
+ # The following config files should be ignored by crun
+ extra = {"runc": "log-file=test-runc.log", "crun": "log-file=test-crun.log"}
+ return _run_cr_test_with_config("annotation", ("annotation.log", "annotation.log"), extra_configs=extra, annotations=annotations)
+
+
</code_context>
<issue_to_address>
**nitpick:** Redundant pre-creation of `annotation.conf` in `test_cr_with_annotation_config` may obscure the behavior being tested
In `test_cr_with_annotation_config`, the call to `_create_criu_config("annotation", "log-file=annotation.log")` is immediately overridden by the `before_checkpoint_cb`/`before_restore_cb` logic in `_run_cr_test_with_config`, which writes a new `annotation.conf` in the temp directory. This makes the initial creation redundant and potentially misleading, since it suggests the test relies on a pre-existing config. Please either remove the initial `_create_criu_config` call or add a brief comment noting that the callbacks always overwrite any existing file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
aa42fab to
7e1befb
Compare
|
@giuseppe @adrianreber I've updated the pull request with support for crun-version of the config file and the possibility to set a config file with container annotation. |
|
TMT tests failed. @containers/packit-build please check. |
56eb200 to
6d0953f
Compare
6d0953f to
e58fea4
Compare
|
@giuseppe Do you have any suggestions where we can add documentation for this functionality? I plan to document this functionality in CRIU's configuration files page but we also have runc checkpoint/restore docs. Would it make sense to add a similar docs in crun? |
e58fea4 to
b33be3f
Compare
|
Could we add it to the man page? |
This commit adds support for specifying a CRIU RPC configuration file. This config file allows users to overwrite the default CRIU options used by the container runtime, for example, to specify options such as `--tcp-established` or `--tcp-close` when checkpointing containers with TCP connections in Kubernetes. For compatibility with runc, the default config file path is set to `/etc/criu/runc.conf`. We also introduce support for crun.conf that will be used instead of runc.conf when the file is available. `criu_set_config_file()` was added to libcriu in version 4.2 Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
This patch adds tests to check support for /etc/criu/runc.conf, /etc/criu/crun.conf, and config file set via container annotation. Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
b33be3f to
1b8e2a3
Compare
|
@giuseppe Thank you for the suggestion! I've added a patch to update the man page. |
giuseppe
left a comment
There was a problem hiding this comment.
I'll let @adrianreber comment on the CRIU pieces.
LGTM
@giuseppe I work with Adrian and have discussed these changes internally but I don't know if he can comment here before next year. |
so let's merge and in case we change it later. Thanks! |
This commit adds support for specifying a CRIU RPC configuration file. This allows users to overwrite the default CRIU options used by the container runtimes, for example, to specify options such as
--tcp-establishedwhen checkpointing containers in Kubernetes. For compatibility with runc, the default config file path is set to/etc/criu/runc.conf.We check for newer CRIU version than 4.1.1 as libcriu doesn't provide
criu_set_config_file()in previous versions: checkpoint-restore/criu#2777Summary by Sourcery
Add support for configuring CRIU via RPC config files, with sensible defaults and annotation-based overrides, and extend checkpoint/restore tests to cover these behaviors.
New Features:
Enhancements:
Tests: